Skip to content

Conversation

lforst
Copy link
Contributor

@lforst lforst commented May 24, 2024

Fixes #12208

The implementation with proxy should be "more" transparent with regards to things like XMLHttpRequest.prototype.open.toString() for example.

Note: Proxies have less support than the previous implementation but it is still within our policy

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's please add a test!

Copy link
Contributor

github-actions bot commented May 24, 2024

size-limit report 📦

⚠️ Warning: Base artifact is not the latest one, because the latest workflow run is not done yet. This may lead to incorrect results. Try to re-run all tests to get up to date results.

Path Size % Change Change
@sentry/browser 22.52 KB +0.08% +17 B 🔺
@sentry/browser (incl. Tracing) 34.85 KB +0.02% +7 B 🔺
@sentry/browser (incl. Tracing, Replay) 71.21 KB +0.02% +11 B 🔺
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 64.48 KB +0.02% +12 B 🔺
@sentry/browser (incl. Tracing, Replay with Canvas) 75.57 KB +0.03% +18 B 🔺
@sentry/browser (incl. Tracing, Replay, Feedback) 88.29 KB +0.01% +8 B 🔺
@sentry/browser (incl. Tracing, Replay, Feedback, metrics) 90.13 KB +0.01% +5 B 🔺
@sentry/browser (incl. metrics) 26.83 KB +0.06% +16 B 🔺
@sentry/browser (incl. Feedback) 39.59 KB +0.06% +23 B 🔺
@sentry/browser (incl. sendFeedback) 27.18 KB +0.07% +17 B 🔺
@sentry/browser (incl. FeedbackAsync) 31.9 KB +0.04% +13 B 🔺
@sentry/react 25.28 KB +0.07% +16 B 🔺
@sentry/react (incl. Tracing) 37.83 KB -0.01% -2 B 🔽
@sentry/vue 26.66 KB +0.02% +5 B 🔺
@sentry/vue (incl. Tracing) 36.68 KB +0.03% +8 B 🔺
@sentry/svelte 22.65 KB +0.06% +12 B 🔺
CDN Bundle 23.76 KB +0.04% +8 B 🔺
CDN Bundle (incl. Tracing) 36.53 KB +0.06% +19 B 🔺
CDN Bundle (incl. Tracing, Replay) 70.86 KB +0.02% +12 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) 76.16 KB +0.01% +7 B 🔺
CDN Bundle - uncompressed 69.61 KB -0.11% -74 B 🔽
CDN Bundle (incl. Tracing) - uncompressed 108.28 KB -0.07% -74 B 🔽
CDN Bundle (incl. Tracing, Replay) - uncompressed 219.66 KB -0.04% -74 B 🔽
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 232.85 KB -0.04% -74 B 🔽
@sentry/nextjs (client) 37.6 KB +0.04% +12 B 🔺
@sentry/sveltekit (client) 35.44 KB -0.02% -5 B 🔽
@sentry/node 115.71 KB - -
@sentry/node - without tracing 90.1 KB - -
@sentry/aws-serverless 99.51 KB - -

View base workflow run

@DerGernTod
Copy link

DerGernTod commented Aug 26, 2024

this fixes half of the issue in that it provides proper names for the xhr prototype functions. however, this is still a culprit:

Function.prototype.toString = function() {
    if (this.__sentry_original__) {
        return this.__sentry_original__.toString();
    }
    return origFunctionToString.apply(this);
}

this implementation per se is fine, unfortunately it conflicts with Function.prototype.name - which is not wrapped like this. so i guess this would also need something like

const origFunctionName = Function.prototype.name;
Object.defineProperty(Function.prototype, "name", {
  get() {
    if (this.__sentry_original__) {
      return this.__sentry_original__.name;
    }
    return origFunctionName.apply(this);
  }
});

this is just a poc, not sure if this code works, but that would bring consistency between Function.prototype.toString and Function.prototype.name i guess
edit: just noticed this doesn't work because the name property is not defined on the prototype, but only on the instance. this makes it a bit more difficult to make the behavior consistent...

@lforst lforst changed the title fix(browser): Don't stomp xhr function names ref(browser): Use Proxy for XHR instrumentation Aug 26, 2024
@lforst
Copy link
Contributor Author

lforst commented Aug 26, 2024

@DerGernTod makes sense. In your original issue, you're describing a symptom rather than the thing you would like to achieve.

I changed the implementation to be based on proxies. I think that should resolve most if not all of your problems. Can you confirm that?

@lforst lforst requested review from AbhiPrasad and mydea August 26, 2024 11:35
@DerGernTod
Copy link

i tried your changes with our tests and looks like it works fine. nice, thanks!

@lforst lforst merged commit 2cbf64e into develop Aug 28, 2024
123 checks passed
@lforst lforst deleted the lforst-xhr-function-names branch August 28, 2024 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Wrapping of Function.prototype.toString causes inconsistent behavior of wrapped functions

4 participants